Skip to content

feat(scan): scan time.Time sets the default decoding #2413

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 7 commits into from
Feb 7, 2023
Merged

feat(scan): scan time.Time sets the default decoding #2413

merged 7 commits into from
Feb 7, 2023

Conversation

monkey92t
Copy link
Collaborator

@monkey92t monkey92t commented Feb 1, 2023 •
edited
Loading

  1. scan time.Time, we should set the default decoding format, because we encoded time.Time in RFC3339Nano format in proto.WriteArg.
case time.Time:
		w.numBuf = v.AppendFormat(w.numBuf[:0], time.RFC3339Nano)
		return w.bytes(w.numBuf)

We should achieve the consistency of Write(Hset Scan) and Read(HMGet Scan):

type data struct {
	Key1 string    `redis:"key1"`
	Key2 int       `redis:"key2"`
	Time time.Time `redis:"time"`
}

// write set
client.HSet(ctx, "hash", &data{
	Key1: "hello",
	Key2: 100,
	Time: time.Now(),
})

// read get
client.HMGet(ctx, "hash", "key1", "key2", "time").Scan(&data)
  1. switch case encoding.BinaryMarshaler is invalid

@monkey92t monkey92t mentioned this pull request Feb 1, 2023
Closed
@monkey92t monkey92t requested a review from vmihailenco February 1, 2023 17:03
@monkey92t monkey92t linked an issue Feb 1, 2023 that may be closed by this pull request
error-redis.Scan(unsupported time.Time) #2380
Closed
@vmihailenco
Copy link
Collaborator

switch case encoding.BinaryMarshaler is invalid

Could you elaborate? I don't see why it should not work...

leoantony72
leoantony72 previously approved these changes Feb 2, 2023
View reviewed changes
@monkey92t
Copy link
Collaborator Author

monkey92t commented Feb 2, 2023 •
edited
Loading

switch case encoding.BinaryMarshaler is invalid

Could you elaborate? I don't see why it should not work...

Oh..it my test error, my mistake, I recover it.

@leoantony72
Copy link

hgetall with Scan command doesn't work if there is time.Time field in the struct and it leads to erratic behaviour like not retrieving every field from the hashset

@monkey92t
Copy link
Collaborator Author

We can directly assert that the encoding.TextUnmarshaler interface, time.Time implements the encoding.TextUnmarshaler interface, which is more convenient. Consider removing Scanner in the future.

@leoantony72
Copy link

That would remove the use of Scan. Which is more reliant

vmihailenco
vmihailenco reviewed Feb 3, 2023
View reviewed changes
vmihailenco
vmihailenco previously approved these changes Feb 3, 2023
View reviewed changes
@chayim chayim added the feature label Feb 7, 2023
@monkey92t monkey92t merged commit 8db6eee into redis:master Feb 7, 2023
@monkey92t monkey92t deleted the scan_time branch February 9, 2023 09:01
@ryan961
Copy link

ryan961 commented Mar 16, 2023

We can directly assert that the encoding.TextUnmarshaler interface, time.Time implements the encoding.TextUnmarshaler interface, which is more convenient. Consider removing Scanner in the future.

hset struct field can assert the encoding.TextMarshaler interface ?
#2161 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chayim chayim chayim approved these changes

@vmihailenco vmihailenco vmihailenco left review comments

@leoantony72 leoantony72 leoantony72 left review comments

Assignees
No one assigned
Labels
feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

error-redis.Scan(unsupported time.Time)
5 participants
@monkey92t @vmihailenco @leoantony72 @ryan961 @chayim